Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Merge all contracts into one ABI #1033

Merged
merged 12 commits into from
Mar 27, 2023
Merged

feat: Merge all contracts into one ABI #1033

merged 12 commits into from
Mar 27, 2023

Conversation

kevaundray
Copy link
Contributor

Related issue(s)

Resolves #

Description

This:

  • Merges all methods in a contract into one ABI.
  • Adds a ContractType to contract methods so we can distinguish between private/public/unconstrained

Summary of changes

Dependency additions / changes

Test additions / changes

Checklist

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt with default settings.
  • I have linked this PR to the issue(s) that it resolves.
  • I have reviewed the changes on GitHub, line by line.
  • I have ensured all changes are covered in the description.

Documentation needs

  • This PR requires documentation updates when merged.

Additional context

@kevaundray
Copy link
Contributor Author

kevaundray commented Mar 23, 2023

Note: We may want to add a test to see what happens when two contracts in different modules are compiled with the same name (This was a concern that was present in the previous implementation)

Copy link
Member

@TomAFrench TomAFrench left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's similar changes which would have to be made to the wasm crate here in order to be consistent.

My preference on this is that we remove support for compiling contracts in TS for now as we're going to be making too many breaking changes. @kobyhallx

crates/noirc_driver/src/contract.rs Outdated Show resolved Hide resolved
crates/nargo/src/cli/compile_cmd.rs Outdated Show resolved Hide resolved
@kevaundray
Copy link
Contributor Author

There's similar changes which would have to be made to the wasm crate here in order to be consistent.

My preference on this is that we remove support for compiling contracts in TS for now as we're going to be making too many breaking changes. @kobyhallx

Ideally we have this for offsite, so they can compile and make tests in typescript. Its not a hard condition and if it makes the lives of tooling folks hard we can find an alternative.

I'm guessing the painpoint you are referring to is when we are developing on master since, once we cut a release the ts and native should be synced up?

crates/nargo/src/cli/fs/program.rs Outdated Show resolved Hide resolved
crates/noirc_driver/src/contract.rs Outdated Show resolved Hide resolved
jfecher and others added 2 commits March 24, 2023 12:51
Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
@TomAFrench
Copy link
Member

Ideally we have this for offsite, so they can compile and make tests in typescript. Its not a hard condition and if it makes the lives of tooling folks hard we can find an alternative.

In that case we need to update noir_wasm so that it returns a similar build artifact for contracts as it will currently return incompatible values relative to the CLI.

@jfecher jfecher enabled auto-merge March 27, 2023 10:57
@jfecher jfecher added this pull request to the merge queue Mar 27, 2023
Merged via the queue into master with commit 473428c Mar 27, 2023
@jfecher jfecher deleted the kw/serialize-contracts branch March 27, 2023 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants